ADFA-4128 (7/11): quickbuild:core — provisioning and the daemon client - #1719

IMPORTANT: the replaced-child identity guard returns before the pending-request cleanup, so a request in flight across a daemon restart is orphaned.

A superseded compile holds requestMutex while awaiting its deferred. A teardown or rebaseline then calls start(), whose shutdown() cannot send the polite SHUTDOWN (the mutex is held), so the child is destroyForcibly()d and process = null runs while the kill is still async. This watcher wakes, sees process !== proc, and returns without completing pending. That compile's deferred is never completed, so it burns the full 300s requestTimeoutMillis still holding the mutex, and the new session's configure blocks behind it for the same 300s. quickbuild/docs/concurrency.md describes this interleaving directly: "a cancelled build's compile still runs to completion unheard - it can delay the next build".

Completing pending here would re-break what the guard fixed (the new session's own configure is in the same map). Make pending per-spawn, the way deliberateStop already is, so each watcher fails only its own child's requests. Not reachable at this commit - nothing constructs the client yet - but it becomes reachable with the session manager in PR 8.

Confirmed, including the mutex hold: the orphaned request burns its full timeout and the next configure queues behind it. Fixing in this stack: the per-spawn state (process, writer, pending, stop marker) becomes one object, so a watcher fails only its own child's requests — deliberateStop already shows the shape.

IMPORTANT: ensureInstalled does blocking file and binder I/O on the caller's dispatcher, with no confinement and no documented threading contract.

isSameContent streams SHA-256 over two full APKs - the candidate and packages.apkFile()'s copy under /data/app - synchronously. awaitStampChange and resolveUid then poll packages.lastUpdateTime/uid, PackageManager binder calls, once a second. Nothing in this class hops to Dispatchers.IO, and the KDoc names no dispatcher the caller must supply.

quickbuild/docs/concurrency.md is explicit that the single session thread every effect runs on may not block: "A blocking call added here stalls the whole session." Hashing a 30 MB APK is hundreds of milliseconds in which the reducer, watcher batch delivery, and generation counter all stop.

Wrap isSameContent and the packages.* reads in withContext(Dispatchers.IO).

Confirmed: no dispatcher hop anywhere in the class, and hashing two APKs on the session thread is exactly what concurrency.md forbids. Fixing in this stack: isSameContent and the packages reads move under Dispatchers.IO, and the KDoc states the confinement.

MINOR: broadcasts.first { } throws when the flow completes without a match, breaking the "never throws" contract stated at line 177.

Flow.first(predicate) raises NoSuchElementException if the flow completes with no matching element. It runs in an async child of the coroutineScope, so that failure cancels the scope - taking stampChanged, the lastUpdateTime fallback that exists precisely for installers which never broadcast - and ensureInstalled throws instead of returning an InstallOutcome.

Unreachable today: nothing constructs ProxyAppInstaller yet, and whether it can fire depends on the app-side adapter that lands later. A callbackFlow closed on receiver unregister completes; a SharedFlow never does. Collecting inside a runCatching and degrading to InstallOutcome.Failed makes the KDoc true either way.

Confirmed; the fallback dying with the scope would be the bad version of ironic. Fixing in this stack: the collection runs in runCatching and degrades to Failed, so the never-throws contract holds for either flow shape.

MINOR: runCatching around a suspend call catches CancellationException, which REVIEW.md section 1 says to rethrow.

launchInstall is suspend, so a cancellation raised inside it is swallowed and reported as InstallOutcome.Failed(InstallCouldNotStart). Line 238 has the same shape.

No user-visible symptom today: the caller's own cancellation also cancels this coroutineScope, which re-raises on exit, so only a CancellationException originating inside launchInstall - its own withTimeout, say - is actually mislabelled. Worth fixing anyway because DaemonProcessClient in this same PR guards the identical pattern three times with catch (e: CancellationException) { throw e }, and a reader carries that expectation across.

try { launchInstall(apk) } catch (e: CancellationException) { throw e } catch (e: Exception) { false }.

Confirmed, at both sites. Fixing in this stack with the explicit CancellationException rethrow, matching the client's three guarded sites.

MINOR: the unchecked mkdirs() turns "the scratch root cannot be created" into a false "not enough storage".

File.getUsableSpace() returns 0 for a path that names no partition, so when root cannot be created the next line reads 0 and this returns NotEnoughStorage(requiredMb = 100, availableMb = 0). prepare checks the shortfall first, so the user is told to free 100 MB on a device with plenty and nothing names the real fault - ScratchDirUnavailable only ever covers the per-project tree, never the root.

Narrow today: root is an app-private noBackupFilesDir subtree where mkdirs essentially always succeeds, and no production code calls this yet. QuickBuildScratchTest and its edge suite cover the blocked-tree case but not a blocked root.

if (!root.isDirectory && !root.mkdirs()) return ScratchDirUnavailable(root.absolutePath).

Confirmed: an uncreatable root reads as a full disk with the wrong remedy on screen. Fixing in this stack with your one-liner, plus the blocked-root test the edge suite is missing.

MINOR: the "exactly one transition means a lone shutdown" rule is asserted here but enforced nowhere.

markIntentionalTransition() is manual, and this class's start and shutdown deliberately never bump. So whether a session restart bumps the epoch once or twice is purely the session manager's convention. If it bumps once for a shutdown-then-start, a stale respawn landing on startEpoch + 1 reads the successor's live daemon as its own zombie and calls shutdown() on the single shared QuickBuildDaemon - leaving the successor holding DaemonReply.Ok while isRunning is false.

Not checkable in this PR; the session manager lands in PR 8. Stating the "a restart must bump twice" requirement in daemonEpoch's KDoc would give that PR's reviewer something concrete to check the manager against.

Confirmed that the convention is load-bearing and unenforced. Adding the "a restart must bump twice" requirement to daemonEpoch's KDoc in this stack so the session-manager PR has a concrete contract to be checked against.

MINOR: the delete-then-rename fallback can destroy the counter it exists to protect.

save's KDoc says the IOException is never swallowed because "losing it would let a later session reuse a generation". But the recovery path deletes the destination first: if delete() succeeds and the retry renameTo still fails, the previously good value is gone, the throw propagates, and the next load() returns null - a fresh session, which is exactly the reuse the doc rules out. The stale .tmp is left behind too.

Both edge tests put a directory at the target, where delete() fails harmlessly and no value was stored anyway, so the file case is unpinned. Reading the old value back before deleting (and restoring it if the retry fails) keeps the invariant the KDoc claims.

Confirmed: the fallback can destroy the value whose loss the KDoc rules out, and the edge tests only cover the directory case. Fixing in this stack: on a failed retry we fall back to a direct write of the new value before throwing — non-atomic beats absent — and the stale tmp is removed; adding the file-at-target test.

MINOR: the code fix at 50-67 is correct, but the file-at-target test you promised is absent, so the arm that preserves the counter is never executed.

Both edge tests put a directory at the target. "save falls back to delete-then-rename when the direct rename is refused" uses an EMPTY directory, where delete() succeeds and the second renameTo lands - control never enters line 58. "save throws when the target cannot be replaced at all" uses a non-empty one, where file.writeText at 59 also fails, so only the throw arm runs. FileGenerationStoreTest's "save overwrites the previous value" does put a plain file there, but on Linux renameTo over a regular file succeeds. So nothing reaches the return at 65 - the whole point of the change.

Add a File subclass whose renameTo always returns false, the way the load test overrides isFile, and assert load() comes back with the new value.

Agreed; test added in 9e833b407. renameTo is called on a file the store constructs internally, so it cannot be overridden from a test. The fixture instead puts an empty directory at the target to defeat the first rename, and uses a file subclass whose delete also removes the staged temp to defeat the retry. That leaves the direct write as the only arm, and the test asserts the counter survives it.

NITPICK: "Bug-12" is a planning code a reader cannot resolve from anything in the repo.

CLAUDE.md's comment rules rule out a comment that needs an external plan document to be understood. Name the behaviour instead - "recorded separately so removed-source assertions do not have to unpick the changed set" - or drop the clause; the field name already says what it holds.

Confirmed; rewording to name the behavior instead of the planning code.

MINOR: load() catches IOException only, so a read that fails any other way does not give the documented "fresh session" - it fails the whole provision.

readText() is readBytes().toString(charset), and readBytes() raises OutOfMemoryError - an Error, not an Exception - on an oversized file. This path is <project>/.androidide/quickbuild/generation under the project root on shared storage, so its size is not app-controlled. The class KDoc promises "a broken state file cannot take quick build down"; what actually happens at the stack tip is that both live call sites (ProxyAppBuildRunner:186, GradleQuickBuildProvisioner:378) sit under an outer catch (Throwable) and report a failed provision instead. A SecurityException takes the same route.

Unreachable while only CoGo writes the store. Bound the read - useLines { it.firstOrNull() } - rather than widening the catch.

The construct, verbatim at 12bd5b4 so the anchor survives line drift:

	override suspend fun load(): Long? =
		withContext(ioDispatcher) {
			try {
				if (file.isFile) file.readText().trim().toLongOrNull() else null
			} catch (e: IOException) {

Real, fixed in ac1cbed38: load() reads the first line only (FileGenerationStore.kt:43). Test only the first line is the counter fails without it with expected: 42 but was : null.

MINOR: the staging path is a single fixed <name>.tmp, but this store is designed to have two writers.

GenerationTracker.adoptAtLeast's KDoc says "The proxy app build stamps its baseline generation through a host-side tracker over the same per-project store", and at the stack tip that is literal: GradleQuickBuildProvisioner:378 opens its own FileGenerationStore.forProject(projectRoot) while ProxyAppBuildRunner:186 opens another on the same path. Both stage through the same generation.tmp, so an interleaved pair can have A's rename land B's value, then B's rename fail, B delete() the file and rewrite - a window with no counter, and no guarantee the larger value survives.

Unreachable today: the provisioner's allocation completes before the session's tracker is opened, and the orchestrator brackets a rebuild. A per-writer suffix removes the shared path.

The construct, verbatim at 12bd5b4 so the anchor survives line drift:

			val tmp = File(file.parentFile, file.name + ".tmp")
			tmp.writeText(generation.toString())
			if (!tmp.renameTo(file)) {

Real, fixed in ac1cbed38: staging uses File.createTempFile (FileGenerationStore.kt:65). Test a save never stages into a file another writer already staged fails without it with FileNotFoundException: .../generation.tmp.

MINOR: configured and scratchFsType are the two pieces of daemon state the per-Spawn refactor left client-level, and each breaks the late-watcher invariant in a different direction.

configured is written here once the identity check at 569 passes - but that check and this write are separate statements, and this runs on Dispatchers.IO. A watcher descheduled between them can pass 569 while S1 is still installed, have a whole start() complete underneath it (shutdown, spawn S2, configure ok, configured = true), then resume and write false. isRunning then reports a healthy, configured S2 as down, and the session respawns a working daemon.

scratchFsType has the opposite gap - not cleared here at all, so it keeps the dead daemon's value while configured is false, against the interface's "null before a successful configure". Harmless today: E2eTimelineRecorder.completed() is its only reader and a daemonDied build never reaches it.

Both go away if the two fields move onto Spawn.

The construct, verbatim at 12bd5b4 so the anchor survives line drift:

			if (this@DaemonProcessClient.spawn !== spawn) {
				log.debug("Replaced quick-build daemon exited with code {}", exitCode)
				return@launch
			}
			configured = false

Real, fixed in c329744a2. configured and scratchFsType are per-Spawn (DaemonProcessClient.kt:110) and isRunning reads the current spawn's (:123); the exit watcher writes nothing. No test pins a deschedule between two statements; verified by reading.

MINOR: a start() cancelled inside configureLocked skips the cleanup shutdown, so the invariant this class states twice does not hold on that path.

request() rethrows CancellationException (the throw e in both its catch arms), so a cancellation during the configure round trip propagates straight out of configureLocked - past the if (outcome !is DaemonReply.Ok) shutdownLocked() at 259-261. This finally then only settles owned(false); nothing kills the child. The KDoc at 134-136 ("with the child shut down first, so a failed start never leaves a daemon behind") and the comment at 255-258 are both false for a cancelled start, and a live unconfigured JVM is left behind.

No caller in this PR can cancel a start. Recovery is plausible but unproven: this.spawn is still assigned, so a later shutdown() would find the child - but the cancellation usually comes from the very teardown that would have made that call. Running shutdownLocked() from this finally when the outcome was not Ok closes it either way.

The construct, verbatim at 12bd5b4 so the anchor survives line drift:

		try {
			return configureLocked(spawn, config)
		} finally {
			// No-op after a configured start; every other way out, cancellation included, tells
			// the watcher this child was never the session's.
			spawn.owned.complete(false)

Real, fixed in c329744a2: a CancellationException out of configureLocked runs shutdownLocked() before rethrowing (DaemonProcessClient.kt:179). Test a start cancelled mid configure kills the child it spawned fails without it with expected to be false.

MINOR: both InstalledPackages reads are unguarded - the sibling site this round's readPackages sweep missed.

ProxyAppInstaller.readPackages exists precisely because "The interface returns null for 'not installed' but does not forbid an implementation throwing", and app-side AndroidInstalledPackages catches only NameNotFoundException - so a binder failure surfaces as a RuntimeException and propagates straight out of quickBuildNeedsConfirm. standardRunNeedsConfirm (48-53) has the same shape. At the stack tip that lands in lifecycleScope.launch(Dispatchers.Main.immediate) at ProjectHandlerActivity:866, which has no catch, so a Quick Build tap becomes a crash report instead of a clobber prompt (REVIEW.md 1).

MINOR because no caller exists in this PR - the same reason this class was graded MINOR in the round-3 thread on line 26. Note the fix is not a bare map-to-null: null for realAppInstalled reads as "not installed" and would clobber without asking. Fail toward asking, the way signingCertSha256's null already means "cannot verify" and makes the provisioner refuse.

The construct, verbatim at 12bd5b4 so the anchor survives line drift:

			RealIdInstall.quickBuildNeedsClobberConfirm(
				realAppInstalled = packages.uid(realApplicationId) != null,
				installedFactory = packages.appComponentFactory(realApplicationId),
			)

Real, fixed in 81f1a7d06: both reads run under failingClosed (QuickBuildClobberCheck.kt:37, :51, :62), which answers true on any non-cancellation failure. Test a PackageManager read that throws asks for confirmation rather than skipping it fails without it with an uncaught IllegalStateException: binder gone.

NITPICK: "Defect T12" is a planning code a reader cannot resolve from anything in the repo - the same finding that was fixed in Fakes.kt this round, at the site the sweep missed.

CLAUDE.md's comment rules rule out shorthand tied to a planning process, and rule out a comment that needs an external plan document to be understood. The rest of the sentence already names the behaviour ("after a CoGo process death the first install's confirm dialog can be lost"), so the prefix carries nothing a reader can use.

Drop the two words. quickbuild/protocol/.../DaemonProtocolDtoTest.kt:71 has the same shape ("pre-Bug-6", "pre-Bug-8") but is outside this diff.

The construct, verbatim at 12bd5b4 so the anchor survives line drift:

			// Defect T12: after a CoGo process death the first install's confirm dialog can
			// be lost - the OS asks, the lifecycle-bound dialog owner is not there to launch

Fixed in c109ee350 (ProxyAppInstallerTest.kt:366). DaemonProtocolDtoTest.kt:71 left alone; it is outside this diff.

NITPICK: this paragraph still reads requestTimeoutMillis as one 300s budget per request, which the client's own KDoc stopped claiming this round.

DaemonProcessClient's class KDoc now says the ceiling is "applied PER PHASE - once to the write and again to the response wait - so one call can hold [requestMutex] for up to twice it", and lines 453 and 482 are two separate withTimeoutOrNull(requestTimeoutMillis). Someone sizing the session thread's worst-case stall from this doc gets 300s where the code allows 600s - and this file is the doc the module points reviewers at for exactly that number.

Still unchanged at the stack tip, where two later commits corrected other parts of this same file. Say "per phase, so up to 600s".

The sentence, verbatim at 12bd5b4:

a request that exceeds requestTimeoutMillis (300 s) comes back as a failed reply rather than an exception

Fixed in c109ee350 (quickbuild/docs/concurrency.md:56): "applied per phase ... so up to 600 s".

NITPICK: timeoutMillis is no longer the whole-install budget - resolveUid was deliberately moved outside it this round.

Line 318 runs resolveUid after withTimeoutOrNull(timeoutMillis) has already returned, and it can spend UID_RETRIES * DEFAULT_POLL_MILLIS (5s) there, so ensureInstalled can take timeoutMillis + 5s. The comment at 312-315 explains why that placement is right; this line still promises a ceiling the code no longer holds, and a caller that sizes its own timeout off it is short by 5s.

Add "plus the bounded uid read that follows it", or name the real ceiling.

The construct, verbatim at 12bd5b4 so the anchor survives line drift:

	/** Whole-install budget, including the time the user spends tapping through dialogs. */
	private val timeoutMillis: Long = DEFAULT_TIMEOUT_MILLIS,

Fixed in c109ee350 (ProxyAppInstaller.kt:172): the KDoc scopes the budget to the verdict and names the uid read as outside it.

NITPICK: this sentence calls the outcome "a retryable failure", which is the opposite of what InstallOutcome.Failed's own KDoc says.

Line 118 defines Failed as "The install could not be completed, and retrying will not help until something changes", explicitly contrasted with ConfirmationNotGiven. A broadcast with no package name is accepted as ours at line 226 and, for a FAILURE status, reaches classify's else arm and comes back as Failed. The distinction is load-bearing, not academic: at the stack tip GradleQuickBuildProvisioner branches on all three arms twice (:126/:132/:140 and :213/:217/:225).

"erring toward a reported failure rather than a false success" says what the code does.

The construct, verbatim at 12bd5b4 so the anchor survives line drift:

 * fallback, which never broadcasts through our receiver. A broadcast with no package name is
 * accepted as ours, erring toward a retryable failure rather than a false success.

Fixed in c109ee350 (ProxyAppInstaller.kt:158): "reported failure".